Better handling around #if conditionals by JonatanWaern · Pull Request #227 · intel/dml-language-server

JonatanWaern · 2026-06-09T07:08:39Z

Refactor how we store object conds for objectdecls
Add logic to select between hashif branches

Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

Copilot

Warning

Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.

Pull request overview

This PR improves handling of #if/#else conditionals during analysis by introducing expression evaluation support and using existence conditions to avoid reporting conflicts across mutually exclusive branches.

Changes:

Added an evaluation module to evaluate a small subset of expressions for conditional existence checks.
Refactored ExistCondition storage to use Arc and added helpers (exists, guaranteed_exists, guaranteed_excluded_from) to reason about conditional branches.
Updated object/spec symbol collection and conflict detection to consider evaluated #if conditions and avoid conflicts between #if and corresponding #else.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 13 comments.

Show a summary per file

File	Description
src/analysis/templating/objects.rs	Uses `ExistCondition` evaluation to filter specs/decls and refines conflict reporting to ignore provably exclusive branches
src/analysis/templating/mod.rs	Exposes the new `evaluation` module
src/analysis/templating/evaluation.rs	Introduces expression evaluation utilities used for `#if` condition resolution
src/analysis/structure/toplevel.rs	Stores conditional stacks in `Arc` and adds existence/exclusion helpers on `ExistCondition`
CHANGELOG.md	Documents improved conflict handling across `#if`/`#else` and built-in version-condition behavior

Suppressed comments (1)

src/analysis/structure/toplevel.rs:97

is_same can return true for different-length conditional stacks because zip() truncates to the shorter iterator (e.g., [A] vs [A,B] will return true). Add an explicit length equality check before the loop (or compare the full vectors) so only identical conditional stacks are treated as the same.

            (ExistCondition::Conditional(selfvec),
             ExistCondition::Conditional(othervec)) => {
                // Currently we cannt check if a condition is equivalent with another,
                // so we will only check if they are literally the same condition expression
                for ((_, cond1),
                     (_, cond2)) in selfvec.iter().zip(othervec.iter()) {
                    if cond1 != cond2 {
                        return false;
                    }
                }
                true
            },

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

-                // same nested hashifs
-                // As it turns out, collision is guaranted regardless of
-                // which branch they are in
+                // Currently we cannt check if a condition is equivalent with another,


Copilot

🟡 Changes recommended

It contains a correctness issue in ExistCondition::guaranteed_overlaps plus several documentation/formatting problems in CHANGELOG that should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Files reviewed: 5/5 changed files
Comments generated: 4
Review effort level: Lite

Copilot

🟡 Changes recommended

The new expression evaluation contains confirmed logic defects that can cause #if conditions to be treated incorrectly, undermining the PR’s core behavioral goal.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

src/analysis/structure/toplevel.rs:90

ExistCondition::guaranteed_overlaps uses zip() without checking that selfvec and othervec have the same length. That can return true when one condition list is a strict prefix of the other (e.g., nested #ifs), incorrectly treating conditions as guaranteed-overlapping and leading to missed/incorrect conflict filtering.

            (ExistCondition::Conditional(selfvec),
             ExistCondition::Conditional(othervec)) => {
                // TODO/NOTE: Currently we cannt check if a condition is equivalent with another,
                // so we will only check if they are literally the same condition expression
                for ((invert1, cond1),

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Lite

Rather speculative for now, as exact future info required to make this choice isn't well-known Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

I don't wanna go _too_ far back with this, but fixed some obviously-visible errors Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

Copilot

🟡 Changes recommended

It introduces a correctness issue in ExistCondition::guaranteed_overlaps (prefix-length conditions can be misclassified as guaranteed overlapping), which should be fixed before merging.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Review details

Suppressed comments (1)

CHANGELOG.md:12

These changelog entries have awkward grammar ("properly not report", "branches on conditions"); tightening the wording will make the release notes clearer.

- The DLS will now properly not report conflicts between statements in a `#if` and its corresponding `#else` branch
- The DLS will now consider all `#if` branches on conditions directly based on `dml_1_2` and `dml_1_4` dead or alive appropriately

Files reviewed: 5/5 changed files
Comments generated: 4
Review effort level: Lite

+                // TODO/NOTE: Currently we cannt check if a condition is equivalent with another,
+                // so we will only check if they are literally the same condition expression
+                for ((invert1, cond1),
+                     (invert2, cond2)) in selfvec.iter().zip(othervec.iter()) {
+                    if cond1 != cond2 || invert1 != invert2 {


+                // Currently we cannt check if a condition is equivalent with another,
+                // so we will only check if they are literally the same condition expression


+// NOTE: 'used' here marks if this type of declaration is the one used for the name
+// of this symbol. Which declaration is used in inferred by the ranking and existconditions
+// of the declarations


-    let mut constants: Vec<Constant> = vec![];
+    let mut constants: Vec<ObjectDecl<Constant>> = vec![];
+
+    // In order to not overly complicate the types, 


JonatanWaern force-pushed the select-hashif-branch branch from 88ae3a0 to 0c8cd42 Compare June 12, 2026 09:22

Refactor how we store object conds for objectdecls

6f749b2

Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

JonatanWaern force-pushed the select-hashif-branch branch from 0c8cd42 to b7a331b Compare September 1, 2026 12:08

JonatanWaern requested review from TSonono and a lite review from Copilot September 1, 2026 12:33

Copilot AI reviewed Sep 1, 2026

View reviewed changes

Copilot started reviewing on behalf of JonatanWaern September 1, 2026 12:58 View session

JonatanWaern force-pushed the select-hashif-branch branch from b7a331b to 21f1f59 Compare September 4, 2026 09:48

JonatanWaern requested a lite review from Copilot September 4, 2026 09:48

Copilot started reviewing on behalf of JonatanWaern September 4, 2026 09:49 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread src/analysis/structure/toplevel.rs

Comment thread CHANGELOG.md

Comment thread CHANGELOG.md Outdated

Comment thread src/analysis/templating/evaluation.rs

JonatanWaern force-pushed the select-hashif-branch branch from 21f1f59 to 877999e Compare September 4, 2026 10:56

JonatanWaern requested a lite review from Copilot September 4, 2026 11:04

Copilot started reviewing on behalf of JonatanWaern September 4, 2026 11:04 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Comment thread src/analysis/templating/evaluation.rs Outdated

Comment thread src/analysis/templating/evaluation.rs

JonatanWaern added 2 commits September 4, 2026 13:27

Add logic to select between hashif branches

e17443f

Rather speculative for now, as exact future info required to make this choice isn't well-known Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

Fixup markdown mistakes in changelog

d275d19

I don't wanna go _too_ far back with this, but fixed some obviously-visible errors Signed-off-by: Jonatan Waern <jonatan.waern@intel.com>

JonatanWaern force-pushed the select-hashif-branch branch from 877999e to d275d19 Compare September 4, 2026 11:27

JonatanWaern requested a lite review from Copilot September 4, 2026 11:33

Copilot started reviewing on behalf of JonatanWaern September 4, 2026 11:33 View session

Copilot AI reviewed Sep 4, 2026

View reviewed changes

Navigation Menu

Uh oh!

Better handling around #if conditionals - #227

Better handling around #if conditionals#227
JonatanWaern wants to merge 3 commits into
mainfrom
select-hashif-branch

JonatanWaern commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		// Currently we cannt check if a condition is equivalent with another,
		// so we will only check if they are literally the same condition expression

Uh oh!

Conversation

JonatanWaern commented Jun 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Suppressed comments (1)

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants